Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fast-generic-serializer supports 'enum' and 'fixed' types #515

Merged

Conversation

krisso-rtb
Copy link
Contributor

@krisso-rtb krisso-rtb commented Sep 12, 2023

PR aims to fix scenario when fast-generic-serializer serializes specific record with fields of type 'enum' and/or 'fixed'.

The 4th commit it the most important one and its message ([fast-avro] Now fast-generic-serializer supports records with 'enum' and 'fixed' types.) might be used after squash.
1st and 2nd commit are self-explanatory, just to avoid pollution of the main commit.
Other commits were pushed just to present differences in the generated code.


Example stacktrace before:

class com.linkedin.avro.fastserde.generated.avro.JustSimpleEnum cannot be cast to class org.apache.avro.generic.GenericData$EnumSymbol (com.linkedin.avro.fastserde.generated.avro.JustSimpleEnum and org.apache.avro.generic.GenericData$EnumSymbol are in unnamed module of loader 'app')
java.lang.ClassCastException: class com.linkedin.avro.fastserde.generated.avro.JustSimpleEnum cannot be cast to class org.apache.avro.generic.GenericData$EnumSymbol (com.linkedin.avro.fastserde.generated.avro.JustSimpleEnum and org.apache.avro.generic.GenericData$EnumSymbol are in unnamed module of loader 'app')
	at com.linkedin.avro.fastserde.generated.serialization.AVRO_1_10.FastSerdeEnums_GenericSerializer_957378572.serializeFastSerdeEnums0(FastSerdeEnums_GenericSerializer_957378572.java:26)
	at com.linkedin.avro.fastserde.generated.serialization.AVRO_1_10.FastSerdeEnums_GenericSerializer_957378572.serialize(FastSerdeEnums_GenericSerializer_957378572.java:19)
	at com.linkedin.avro.fastserde.generated.serialization.AVRO_1_10.FastSerdeEnums_GenericSerializer_957378572.serialize(FastSerdeEnums_GenericSerializer_957378572.java:11)
	at com.linkedin.avro.fastserde.FastGenericSerializerGeneratorTest.dataAsBinaryDecoder(FastGenericSerializerGeneratorTest.java:615)

with enum and fixed fields.
For now marked as ignored e.g. due to:

class com.linkedin.avro.fastserde.generated.avro.JustSimpleEnum
cannot be cast to class org.apache.avro.generic.GenericData$EnumSymbol
@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (b5f3ce6) 45.82% compared to head (416f4e3) 45.83%.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #515   +/-   ##
=========================================
  Coverage     45.82%   45.83%           
- Complexity     4441     4442    +1     
=========================================
  Files           398      398           
  Lines         28040    28040           
  Branches       4622     4622           
=========================================
+ Hits          12850    12851    +1     
  Misses        13634    13634           
+ Partials       1556     1555    -1     

see 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@FelixGV FelixGV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks a lot for this contribution!

The main change LGTM, but I have a concern about one of the "cleanups" in the first comment... can you elaborate on that one?

Comment on lines -304 to +301
ifBlock = ifBlock != null ? ifBlock._elseif(condition) : body._if(condition);
ifBlock = body._if(condition);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the rationale for this change?

Copy link
Contributor Author

@krisso-rtb krisso-rtb Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's consider two key facts:

  • For the first time we enter this block ifBlock is null
  • The block can be entered at most once due to break at the end.

It means condition ifBlock != null is always false.
"Always" in this context means "at most once".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That's right... I missed that detail when I refactored this code in 608bce8

Copy link
Collaborator

@FelixGV FelixGV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines -304 to +301
ifBlock = ifBlock != null ? ifBlock._elseif(condition) : body._if(condition);
ifBlock = body._if(condition);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That's right... I missed that detail when I refactored this code in 608bce8

@FelixGV FelixGV merged commit 3d8a30b into linkedin:master Sep 14, 2023
2 checks passed
@krisso-rtb krisso-rtb deleted the fast-generic-serializer-enums-fixed-v2 branch September 14, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants